Skip to content

feat(accesscontrol): add external setRoleAdmin and grantRoleBatch/rev…#156

Merged
adamgall merged 4 commits into
Perfect-Abstractions:mainfrom
isihin-3:feat/accesscontrol-phase1-core
Oct 31, 2025
Merged

feat(accesscontrol): add external setRoleAdmin and grantRoleBatch/rev…#156
adamgall merged 4 commits into
Perfect-Abstractions:mainfrom
isihin-3:feat/accesscontrol-phase1-core

Conversation

@isihin-3

@isihin-3 isihin-3 commented Oct 30, 2025

Copy link
Copy Markdown
Contributor

…okeRoleBatch

Summary

Adds Phase 1 usability improvements to AccessControlFacet : an external setRoleAdmin with admin auth, plus grantRoleBatch and revokeRoleBatch for efficient DAO-scale role management. This enables dynamic role hierarchy updates and multi-address role operations in a single transaction.

Changes Made

AccessControlFacet

  • Implemented setRoleAdmin(bytes32 _role, bytes32 _adminRole) with current-admin authorization and RoleAdminChanged event.
  • Implemented grantRoleBatch(bytes32 _role, address[] calldata _accounts) with admin authorization and per-account RoleGranted events.
  • Implemented revokeRoleBatch(bytes32 _role, address[] calldata _accounts) with admin authorization and per-account RoleRevoked events.
  • Batch ops skip redundant writes and avoid duplicate events.

Tests

  • Added coverage for setRoleAdmin success/revert paths.
  • Added coverage for batch grant/revoke, including partial duplicates and event expectations.
  • Verified default admin behaviors and role hierarchy interactions.

Checklist

Before submitting this PR, please ensure:

  • [ x] Code follows the Solidity feature ban - No inheritance, constructors, modifiers, public/private variables, external library functions, using for directives, or selfdestruct

  • [x ] Code follows Design Principles - Readable, uses diamond storage, favors composition over inheritance

  • [x ] Code matches the codebase style - Consistent formatting, documentation, and patterns (e.g. ERC20Facet.sol)

  • [x ] Code is formatted with forge fmt

  • [ x] Tests are included - All new functionality has comprehensive tests

  • [ x] All tests pass - Run forge test and ensure everything works

  • [ x] Documentation updated - If applicable, update relevant documentation

Additional Notes

  • DEFAULT_ADMIN_ROLE remains initial admin for new roles. setRoleAdmin enables controlled migration to custom hierarchies.
  • This PR closes Phase 1 of Issue #[149].
  • The logic for the batch functions intentionally "repeats" the logic from the single functions to adhere to the 'Read First' and 'self-contained' design principles.

@github-actions

github-actions Bot commented Oct 30, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Coverage Report

Coverage

Metric Coverage Details
Lines 46% 516/1123 lines
Functions 61% 123/203 functions
Branches 30% 56/184 branches

Last updated: Fri, 31 Oct 2025 14:01:24 GMT for commit f8cf561

@github-actions

github-actions Bot commented Oct 30, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Gas Report

Comparing gas usage between main and feat/accesscontrol-phase1-core

Summary

  • Optimized: 14 functions (🟢 -520 gas)
  • Increased: 27 functions (🔴 +1,400 gas)
  • Net Change: 🔴 +880 gas

Details

Contract Function Before After Change
Test testFuzz_HasRole_ConsistentAcrossMultipleRoles() N/A N/A 🔴 +280
Test test_ComplexRoleHierarchy_GrantingThroughHierarchy() N/A N/A 🔴 +137
Test test_RenounceRole_CanRenounceMultipleRoles() N/A N/A 🔴 +128
Test test_HasRole_HandlesMultipleRolesPerAccount() N/A N/A 🔴 +92
Test test_HasRole_HandlesMultipleAccountsPerRole() N/A N/A 🔴 +91
Test test_RenounceRole_SucceedsForOwnRole() N/A N/A 🔴 +72
Test test_StorageConsistency_HasRoleMatchesStorage() N/A N/A 🔴 +68
Test test_EdgeCase_RoleAdminOfItself() N/A N/A 🟢 -65
Test test_RevokeRole_SucceedsWithCustomRoleAdmin() N/A N/A 🟢 -65
Test test_RevokeRole_CanRevokeFromZeroAddress() N/A N/A 🔴 +56
Test test_RevertWhen_GrantRole_CallerIsNotAdmin() N/A N/A 🔴 +55
Test testFuzz_RoleAdminHierarchy() N/A N/A 🔴 +47
Test test_RenounceRole_CanRenounceDefaultAdminRole() N/A N/A 🔴 +46
Test test_HasRole_ReturnsCorrectInitialState() N/A N/A 🔴 +46
Test testFuzz_RenounceRole_OnlyAccountCanRenounce() N/A N/A 🟢 -44
Test test_RevertWhen_RequireRole_AccountDoesNotHaveRole() N/A N/A 🟢 -44
Test test_GetRoleAdmin_ReturnsDefaultAdminForNewRole() N/A N/A 🟢 -44
Test test_EdgeCase_CircularRoleAdminHierarchy() N/A N/A 🟢 -43
Test test_HasRole_ReturnsTrueForGrantedRole() N/A N/A 🟢 -43
Test test_GrantRole_CanGrantToSelf() N/A N/A 🟢 -43

Showing top 20 changes out of 41 total.

View all 41 changes
Contract Function Before After Change
Test testFuzz_HasRole_ConsistentAcrossMultipleRoles() N/A N/A 🔴 +280
Test test_ComplexRoleHierarchy_GrantingThroughHierarchy() N/A N/A 🔴 +137
Test test_RenounceRole_CanRenounceMultipleRoles() N/A N/A 🔴 +128
Test test_HasRole_HandlesMultipleRolesPerAccount() N/A N/A 🔴 +92
Test test_HasRole_HandlesMultipleAccountsPerRole() N/A N/A 🔴 +91
Test test_RenounceRole_SucceedsForOwnRole() N/A N/A 🔴 +72
Test test_StorageConsistency_HasRoleMatchesStorage() N/A N/A 🔴 +68
Test test_EdgeCase_RoleAdminOfItself() N/A N/A 🟢 -65
Test test_RevokeRole_SucceedsWithCustomRoleAdmin() N/A N/A 🟢 -65
Test test_RevokeRole_CanRevokeFromZeroAddress() N/A N/A 🔴 +56
Test test_RevertWhen_GrantRole_CallerIsNotAdmin() N/A N/A 🔴 +55
Test testFuzz_RoleAdminHierarchy() N/A N/A 🔴 +47
Test test_RenounceRole_CanRenounceDefaultAdminRole() N/A N/A 🔴 +46
Test test_HasRole_ReturnsCorrectInitialState() N/A N/A 🔴 +46
Test testFuzz_RenounceRole_OnlyAccountCanRenounce() N/A N/A 🟢 -44
Test test_RevertWhen_RequireRole_AccountDoesNotHaveRole() N/A N/A 🟢 -44
Test test_GetRoleAdmin_ReturnsDefaultAdminForNewRole() N/A N/A 🟢 -44
Test test_EdgeCase_CircularRoleAdminHierarchy() N/A N/A 🟢 -43
Test test_HasRole_ReturnsTrueForGrantedRole() N/A N/A 🟢 -43
Test test_GrantRole_CanGrantToSelf() N/A N/A 🟢 -43
Test test_RevokeRole_SucceedsWithDefaultAdmin() N/A N/A 🔴 +36
Test test_ComplexRoleHierarchy_CannotGrantWithoutProperAdmin() N/A N/A 🔴 +34
Test testFuzz_GrantRole_OnlyAdminCanGrant() N/A N/A 🔴 +33
Test test_ComplexRoleHierarchy_Setup() N/A N/A 🔴 +23
Test test_GrantRole_DoesNotEmitEventWhenAlreadyGranted() N/A N/A 🔴 +23
Test test_GrantRole_CanGrantToZeroAddress() N/A N/A 🔴 +23
Test test_StorageConsistency_RoleAdminMatchesStorage() N/A N/A 🔴 +23
Test test_HasRole_ReturnsFalseForNonGrantedRole() N/A N/A 🔴 +23
Test test_StorageSlot_UsesCorrectPosition() N/A N/A 🔴 +23
Test testFuzz_RevokeRole_OnlyAdminCanRevoke() N/A N/A 🟢 -22
Test test_RevertWhen_RenounceRole_CallerIsNotAccount() N/A N/A 🟢 -22
Test test_RevertWhen_RequireRole_ZeroAddressDoesNotHaveRole() N/A N/A 🟢 -22
Test test_GetRoleAdmin_DefaultAdminRoleAdminIsItself() N/A N/A 🔴 +22
Test test_GrantRole_SucceedsWithCustomRoleAdmin() N/A N/A 🟢 -21
Test test_RevertWhen_RevokeRole_CallerIsNotCustomAdmin() N/A N/A 🟢 -21
Test test_GrantRole_SucceedsWithDefaultAdmin() N/A N/A 🟢 -21
Test test_RevertWhen_GrantRole_CallerIsNotCustomAdmin() N/A N/A 🔴 +11
Test test_EdgeCase_MultipleOperationsOnSameRole() N/A N/A 🔴 +4
Test test_GetRoleAdmin_ReturnsCorrectAdminAfterChange() N/A N/A 🔴 +2
Test test_RevokeRole_DoesNotEmitEventWhenNotGranted() N/A N/A 🔴 +1
Test test_RenounceRole_DoesNotEmitEventWhenNotGranted() N/A N/A 🔴 +1
ℹ️ About this report

This report compares gas usage between the base branch and this PR using forge snapshot.

  • 🟢 indicates a gas improvement (reduction)
  • 🔴 indicates a gas regression (increase)
  • Functions not shown have unchanged gas costs

To run this locally:

# Generate snapshot for current branch
forge snapshot

# Compare with another branch
git checkout main
forge snapshot --diff .gas-snapshot

Last updated: Fri, 31 Oct 2025 14:01:47 GMT for commit f8cf561

adamgall
adamgall suggested changes Oct 30, 2025
View reviewed changes
Comment thread .gas-snapshot Outdated
@isihin-3 isihin-3 requested a review from adamgall October 31, 2025 12:24
@adamgall adamgall linked an issue Oct 31, 2025 that may be closed by this pull request
[Feature]: AccessControl Library Enhancements - setRoleAdmin, Batch Operations #149
Closed
adamgall
adamgall approved these changes Oct 31, 2025
View reviewed changes

@adamgall adamgall left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks @isihin-3 !

@adamgall adamgall merged commit ba4ff30 into Perfect-Abstractions:main Oct 31, 2025
4 checks passed
JackieXu pushed a commit to JackieXu/Compose that referenced this pull request Nov 6, 2025
@adamgall
Merge pull request Perfect-Abstractions#156 from isihin-3/feat/access…
57e2103 
…control-phase1-core

feat(accesscontrol): add external setRoleAdmin and grantRoleBatch/rev…
@isihin-3 isihin-3 deleted the feat/accesscontrol-phase1-core branch November 9, 2025 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maxnorm maxnorm maxnorm left review comments

+1 more reviewer

@adamgall adamgall adamgall approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature]: AccessControl Library Enhancements - setRoleAdmin, Batch Operations

3 participants

@isihin-3 @adamgall @maxnorm